Fix issues reported by Errorprone static analysis tool #12419
Conversation
Pearl1594
requested review from
DaanHoogland,
Copilot,
sureshanaparti and
weizhouapache
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12419 +/- ##
============================================
+ Coverage 16.23% 16.25% +0.01%
- Complexity 13382 13390 +8
============================================
Files 5657 5657
Lines 498999 499060 +61
Branches 60566 60613 +47
============================================
+ Hits 81035 81112 +77
+ Misses 408928 408904 -24
- Partials 9036 9044 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR integrates Google Error Prone into the build system to perform compile-time static analysis. Error Prone has identified and the PR fixes multiple categories of issues including:
- Bugs where methods were called but results were ignored
- Incorrect primitive comparisons that should use
.equals() - Malformed logging statements with wrong placeholders
- Dead code and redundant operations
- Missing
hashCode()implementations whenequals()is overridden
Changes:
- Added Error Prone (version 2.24.1) to the Maven compiler plugin configuration
- Fixed 35+ Error Prone violations across multiple modules including server, engine, plugins, and core components
- Applied
@SuppressWarnings("BanJNDI")annotations to LDAP-related code where JNDI usage is intentional
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Added Error Prone plugin configuration to Maven compiler |
| ByteBuffer.java | Fixed missing assignment of Arrays.copyOf result |
| ServerNtlmsspChallenge.java | Fixed array concatenation in error message |
| GlobalLoadBalancingRulesServiceImpl.java | Changed == to .equals() for Long comparison |
| RoutedIpv4ManagerImpl.java | Fixed missing format placeholders in error messages |
| UserVmManagerImpl.java | Fixed config key retrieval and enum comparison |
| VolumeApiServiceImpl.java | Removed unnecessary String.format wrapper |
| ManagementServerImpl.java | Changed == to .equals() for Long comparison |
| IpAddressManagerImpl.java | Fixed missing throw keyword for exception |
| HighAvailabilityManagerImpl.java | Fixed logging format with correct placeholders |
| ConfigurationManagerImpl.java | Changed == to .equals() for Boolean comparison |
| TemplateJoinDaoImpl.java | Used saturated cast to prevent overflow |
| Argument.java | Improved Comparable implementation with proper typing |
| ApiXmlDocWriter.java | Fixed class type checking logic |
| OpenLdapUserManagerImpl.java | Fixed logging and added JNDI suppression |
| ADLdapUserManagerImpl.java | Added JNDI suppression annotation |
| StorPoolDataMotionStrategy.java | Removed unnecessary String.format wrapper |
| StorPoolPrimaryDataStoreDriver.java | Changed == to .equals() for Long comparison |
| NexentaStorAppliance.java | Added missing hashCode() implementations |
| RedfishWrapper.java | Fixed incorrect format placeholder count |
| XenServerGuru.java | Fixed typo and simplified Pair construction |
| MultipathSCSIAdapterBase.java | Converted logging to use placeholders |
| KVMStorageProcessor.java | Fixed missing throw keywords |
| LibvirtComputingResource.java | Fixed logging format placeholders |
| HypervInvestigator.java | Simplified boolean return expression |
| RootCAProvider.java | Removed duplicate method call |
| OnwireClassRegistry.java | Fixed self-assignment bug |
| DefaultEndPointSelector.java | Modernized iterator removal pattern |
| ScaleIOVMSnapshotStrategy.java | Changed == to .equals() for Long comparison |
| Upgrade41500to41510.java | Replaced anonymous HashMap with Map.of() |
| DatabaseAccessObject.java | Fixed logging format mismatch |
| SystemVmTemplateRegistration.java | Replaced anonymous HashMap with Map.of() |
| NetworkOfferingVO.java | Fixed incorrect hardcoded enum value |
| NetworkOrchestrator.java | Removed duplicate condition check |
| VirtualMachineManagerImpl.java | Fixed missing format placeholder |
| DirectAgentAttache.java | Added missing hashCode() implementation |
| AgentAttache.java | Added missing hashCode() implementation |
| DirectDownloadCommand.java | Removed dead assignment |
| RequestWrapper.java | Fixed incorrect .getClass() call |
| HAProxyConfigurator.java | Removed unnecessary toString() call |
| AbstractConfigItemFacade.java | Fixed incorrect .getClass() usage |
| UpdateBackupOfferingCmd.java | Fixed incomplete error message |
| MockVmMgr.java | Fixed modulo operation for bounded random |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("BanJNDI") |
There was a problem hiding this comment.
The @SuppressWarnings(\"BanJNDI\") annotation is being used to suppress Error Prone warnings about JNDI usage. While JNDI usage in LDAP operations is legitimate, ensure that all JNDI contexts are properly secured and validated to prevent LDAP injection attacks. Review the input validation in these methods (getUsersInGroup, getUserForDn, searchUser, searchUsers) to ensure user-controlled data is properly sanitized before being used in LDAP queries.
| private static final String MICROSOFT_AD_MEMBERS_FILTER = "memberOf"; | ||
|
|
||
| @Override | ||
| @SuppressWarnings("BanJNDI") |
There was a problem hiding this comment.
The @SuppressWarnings(\"BanJNDI\") annotation suppresses JNDI-related warnings. Ensure that the groupName parameter and any other user-controlled inputs are properly validated and sanitized before being used in LDAP queries to prevent LDAP injection vulnerabilities.
| public int hashCode() { | ||
| return 1; |
There was a problem hiding this comment.
The hashCode() implementation for LuParams returns a constant value of 1, which violates the hashCode contract. Since the equals() method considers all instances of LuParams to be equal (line 255), this constant hashCode is technically correct but indicates a potential design issue. If all instances are equal, consider whether this class should be used as a key in hash-based collections, or if the equality logic needs to be refined.
engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java
Outdated
Show resolved
Hide resolved
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentAttache.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16354 |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Pearl1594
changed the title
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16362 |
|
3 participants
Description
This PR adds error prone to the build. All issues identified by it have been addressed.
Issue were identified at compile time :
mvn clean compiletemporarily added the following change in pom.xml
The project built successfully:
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?